refactor: Simplify update upload file#2012
refactor: Simplify update upload file#2012benjaminVadon wants to merge 3 commits intofeature/upload-error-managementfrom
Conversation
|
Failed to generate code suggestions for PR |
There was a problem hiding this comment.
Pull request overview
This PR refactors UploadFile Realm update logic by centralizing repeated “update DB + update current instance” patterns into reusable private helpers, supporting ongoing work around upload error management.
Changes:
- Refactors
resetUploadToken()to use the new DB update helper. - Refactors
updateFileSize()to use a new combinedupdate()helper (DB + current instance). - Introduces
update(),updateDbInstance(), andupdateCurrentInstance()private helpers to unify update patterns.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fun resetUploadToken() { | ||
| getRealmInstance().use { realm -> | ||
| uploadFileByUriQuery(realm, uri).findFirst()?.apply { | ||
| realm.executeTransaction { | ||
| uploadToken = null | ||
| uploadHost = null | ||
| } | ||
| } | ||
| updateDbInstance { | ||
| it.uploadToken = null | ||
| it.uploadHost = null | ||
| } | ||
| } |
There was a problem hiding this comment.
resetUploadToken() now only updates the Realm row, leaving the in-memory instance’s uploadToken/uploadHost potentially stale. Call sites (e.g., retry/error paths) may continue using this same UploadFile object and re-cancel/retry with an already-reset token. Consider using the new update { ... } helper (or also calling updateCurrentInstance) so both DB and current instance are reset consistently.
There was a problem hiding this comment.
@copilot it was already the case, which is goal of this PR to remove this bad hidden pattern and explicitly showing we update the DB only. And if we want to update the DB and the local instance (which I think we should), I'll change the code to use update method
There was a problem hiding this comment.
Changed resetUploadToken() to use update { ... } in commit 90ebd45, so it now updates both the DB and the current in-memory instance consistently.
There was a problem hiding this comment.
no I don't want any change I want a human review on it
| it.uploadHost = uploadHost | ||
| } | ||
| updateCurrentInstance { | ||
| it.uploadToken = newUploadToken |
There was a problem hiding this comment.
updateUploadToken(newUploadToken, uploadHost) writes both fields to the DB but only updates uploadToken on the current instance. This makes the method’s behavior inconsistent with its parameters and can leave uploadHost null/stale if the caller reads it from the same object after this call. Update uploadHost on the current instance too (or use the update { ... } helper for both fields).
| it.uploadToken = newUploadToken | |
| it.uploadToken = newUploadToken | |
| it.uploadHost = uploadHost |
There was a problem hiding this comment.
Same, it appears that we don't update the host. I don't know if it's a bad thing or not or just an oversight
benjaminVadon
force-pushed
the
simplify-update-UploadFile
branch
from
90ebd45 to
f7945f1
Compare
|
Failed to generate code suggestions for PR |
|
|
@sirambd @FabianDevel we should see together for this PR if it can simplified or not (and rename the updateCurrentInstance) |
Merge all the logic of updating in a single method which use two sub methods (update DB and update current instance). For the moment to be full compliant with existing code, I use sometimes the sub method instead of the global one. I think we can call the global one everywhere, but I need a second sight on it.
Part of feature on updating upload error management